Skip to content

IPA uses srs#205

Merged
arijitdutta67 merged 1 commit intomasterfrom
ad/ipa-uses-srs
Mar 10, 2023
Merged

IPA uses srs#205
arijitdutta67 merged 1 commit intomasterfrom
ad/ipa-uses-srs

Conversation

@arijitdutta67
Copy link
Copy Markdown

@arijitdutta67 arijitdutta67 commented Mar 7, 2023 •
edited
Loading

Description

This PR enables the IPA scheme to use kzg commitment key. This helps the scheme to use kzg srs elements as the set of generators. It also enables the scheme to use a single pippenger_runtime_state for performing all multi-scalar-multiplications. In particular, it resolves the issues #163 , #164.

Checklist:

  • I have reviewed my diff in github, line by line.
  • Every change is related to the PR description.
  • I have linked this pull request to the issue(s) that it resolves.
  • There are no unexpected formatting changes, superfluous debug logs, or commented-out code.
  • There are no circuit changes, OR specifications in /markdown/specs have been updated.
  • There are no circuit changes, OR a cryptographer has been assigned for review.
  • I've updated any terraform that needs updating (e.g. environment variables) for deployment.
  • The branch has been rebased against the head of its merge target.
  • I'm happy for the PR to be merged at the reviewer's next convenience.
  • New functions, classes, etc. have been documented according to the doxygen comment format. Classes and structs must have @brief describing the intended functionality.
  • If existing code has been modified, such documentation has been added or updated.

@codygunton codygunton self-requested a review March 7, 2023 13:47
@codygunton codygunton added this to the IPA Commitment scheme milestone Mar 7, 2023
@codygunton codygunton linked an issue Mar 7, 2023 that may be closed by this pull request
IPA uses SRS #163
Closed
@arijitdutta67 arijitdutta67 marked this pull request as ready for review March 7, 2023 16:19
@arijitdutta67 arijitdutta67 linked an issue Mar 7, 2023 that may be closed by this pull request
Share pippenger_runtime_state. #164
Closed
codygunton
codygunton suggested changes Mar 7, 2023
View reviewed changes
Comment thread cpp/src/aztec/honk/pcs/commitment_key.hpp
Comment thread cpp/src/aztec/honk/pcs/ipa/ipa.hpp Outdated
Comment thread cpp/src/aztec/honk/pcs/ipa/ipa.test.cpp Outdated
@codygunton
Copy link
Copy Markdown

codygunton commented Mar 7, 2023

Regarding your points:

I had to make srs and pippenger_runtime_state public inside commitment_key.hpp to use them inside IPA. Would like to have a discussion if that is concerning.

I left a suggestion for this in my review.

The srs is not generic. For now it seems ok to me as we are not going to use the grumpkin srs immediately. But I wish to know if some improvement on this is possible with minimal changes.

The aforementioned suggestion should also handle this. The commitment key is a flexible parameter that can contain/point to whatever sort of SRS you like.

I have kept the commit function inside IPA unlike other pcs as it uses pippenger_without_endomorphism_basis_points method instead of pippengen_unsafe method.

I would prefer that we keep the same structure as in honk/pcs/kzg, unless there's a good reason for departing from what's there. I think your point is that we are somehow not obligated to insulate the user from a call to an unsafe Pippenger method? This to me doesn't seem like a good reason to use different interfaces.

@arijitdutta67
Copy link
Copy Markdown
Author

arijitdutta67 commented Mar 9, 2023

Regarding your points:

I had to make srs and pippenger_runtime_state public inside commitment_key.hpp to use them inside IPA. Would like to have a discussion if that is concerning.

I left a suggestion for this in my review.

The srs is not generic. For now it seems ok to me as we are not going to use the grumpkin srs immediately. But I wish to know if some improvement on this is possible with minimal changes.

The aforementioned suggestion should also handle this. The commitment key is a flexible parameter that can contain/point to whatever sort of SRS you like.

I have kept the commit function inside IPA unlike other pcs as it uses pippenger_without_endomorphism_basis_points method instead of pippengen_unsafe method.

I would prefer that we keep the same structure as in honk/pcs/kzg, unless there's a good reason for departing from what's there. I think your point is that we are somehow not obligated to insulate the user from a call to an unsafe Pippenger method? This to me doesn't seem like a good reason to use different interfaces.

Thanks Cody for you timely suggestions which helped me a lot. I have tried my best to adhere to your suggestions. Please let me know if you have any more concerns.

@arijitdutta67 arijitdutta67 requested a review from codygunton March 9, 2023 12:22
codygunton
codygunton previously requested changes Mar 9, 2023
View reviewed changes
Copy link
Copy Markdown

@codygunton codygunton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You raised the following issues in our review call:

  • You were unsure whether it is secure to use unsafe Pippenger methods here.
  • You were concerned that it may be inefficient to use the same Pippenger runtime state for different sets of generators and different MSM lengths.

Will you briefly look into these questions, if you haven't, and make github issues as appropriate?

Comment thread cpp/src/aztec/honk/pcs/commitment_key.hpp
Comment thread cpp/src/aztec/honk/pcs/ipa/ipa.test.cpp Outdated
Comment thread cpp/src/aztec/honk/pcs/ipa/ipa.test.cpp Outdated
This was referenced Mar 10, 2023
Closed
Closed
@arijitdutta67
Copy link
Copy Markdown
Author

arijitdutta67 commented Mar 10, 2023

You raised the following issues in our review call:

  • You were unsure whether it is secure to use unsafe Pippenger methods here.
  • You were concerned that it may be inefficient to use the same Pippenger runtime state for different sets of generators and different MSM lengths.

Will you briefly look into these questions, if you haven't, and make github issues as appropriate?

Thanks. Created the issue #237 for this.

@arijitdutta67
grumpkin srs read function works
78a1b00 
ipa uses kzg params

commit function is reinstated inside ipa

commenting out grumpkin srs test as we are not using them now

cleanup before review

more cleanup

using separate ipa params (wip)

resolve code duplication and other issues pointed by Cody

fixing rebase bug

minor cleanup

cleanup suggested by Cody
@arijitdutta67 arijitdutta67 dismissed codygunton’s stale review March 10, 2023 09:18

I have addressed all of Cody's suggestions and we decided to merge this PR in our review call.

@arijitdutta67 arijitdutta67 merged commit 1c002af into master Mar 10, 2023
@arijitdutta67 arijitdutta67 deleted the ad/ipa-uses-srs branch March 10, 2023 09:20
codygunton
codygunton reviewed Mar 20, 2023
View reviewed changes
Comment thread cpp/src/barretenberg/honk/pcs/commitment_key.hpp
codygunton
codygunton reviewed Mar 20, 2023
View reviewed changes
Comment thread cpp/src/barretenberg/honk/pcs/commitment_key.hpp
@arijitdutta67 arijitdutta67 mentioned this pull request Mar 21, 2023
Closed
11 tasks
ludamad pushed a commit to AztecProtocol/aztec-packages that referenced this pull request Jul 22, 2023
@arijitdutta67
IPA uses SRS (AztecProtocol/barretenberg#205)
2ebfd14 
ipa uses kzg params

commit function is reinstated inside ipa

commenting out grumpkin srs test as we are not using them now

cleanup before review

more cleanup

using separate ipa params (wip)

resolve code duplication and other issues pointed by Cody

fixing rebase bug

minor cleanup

cleanup suggested by Cody
ludamad pushed a commit to AztecProtocol/aztec-packages that referenced this pull request Jul 24, 2023
@arijitdutta67
IPA uses SRS (AztecProtocol/barretenberg#205)
2106886 
ipa uses kzg params

commit function is reinstated inside ipa

commenting out grumpkin srs test as we are not using them now

cleanup before review

more cleanup

using separate ipa params (wip)

resolve code duplication and other issues pointed by Cody

fixing rebase bug

minor cleanup

cleanup suggested by Cody
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@codygunton codygunton codygunton left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@arijitdutta67 arijitdutta67

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Share pippenger_runtime_state. IPA uses SRS

2 participants

@arijitdutta67 @codygunton